-
Notifications
You must be signed in to change notification settings - Fork 369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix UI tests and make connection view releasable #7453
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 30 files reviewed, 3 unresolved discussions
ios/MullvadVPN/View controllers/Tunnel/TunnelViewController.swift
line 1 at r1 (raw file):
//
This file has been replaced by FI_TunnelViewController (that has in turn been deleted), but for some reason we get a diff... 🫠 Well, doesn't hurt to review again, I guess.
ios/MullvadVPN.xcodeproj/project.pbxproj
line 2699 at r1 (raw file):
children = ( F0ADF1CF2D01B50B00299F09 /* ChipView */, 7AA130972CFF364F00640DF9 /* FeatureIndicators */,
Folder on top, hence the non-alphabetical sorting.
ios/MullvadVPN.xcodeproj/project.pbxproj
line 3050 at r1 (raw file):
isa = PBXGroup; children = ( 4419AA862D28264D001B13C9 /* ConnectionView */,
Folder on top, hence the non-alphabetical sorting.
fc7f461
to
1b78c30
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 27 of 30 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @rablador)
ios/MullvadVPN/Classes/AccessbilityIdentifier.swift
line 160 at r2 (raw file):
case connectionPanelOutAddressRow case connectionPanelOutIpv6AddressRow case connectionPanelServer
Nit: perhaps this should state what type of element it is, as in connectionPanelServerLabel
or similar
1b78c30
to
df25524
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 29 of 30 files reviewed, 3 unresolved discussions (waiting on @acb-mv)
ios/MullvadVPN/Classes/AccessbilityIdentifier.swift
line 160 at r2 (raw file):
Previously, acb-mv wrote…
Nit: perhaps this should state what type of element it is, as in
connectionPanelServerLabel
or similar
Done,.
1281fef
to
f92f71f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 26 of 30 files reviewed, 4 unresolved discussions (waiting on @acb-mv)
ios/MullvadVPN/View controllers/Tunnel/ConnectionView/ConnectionViewViewModel.swift
line 27 at r3 (raw file):
} @Published var tunnelStatus: TunnelStatus { didSet {
Opportunistic fix to make sure we don't populate outgoing address labels with old data.
f92f71f
to
bff87d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 22 of 30 files at r1, 3 of 3 files at r2, 4 of 4 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @rablador)
ios/MullvadVPN/View controllers/Tunnel/ConnectionView/ConnectionViewViewModel.swift
line 27 at r3 (raw file):
Previously, rablador (Jon Petersson) wrote…
Opportunistic fix to make sure we don't populate outgoing address labels with old data.
nit
We had a team agreement in the past that we should refrain from doing these kind of things as it quickly make the codebase harder to read and to navigate in.
I think it's okay for now, because there's only 1 layer of indirection, but I would like to encourage us to find a better way to do this next time.
ios/MullvadVPNUITests/RelayTests.swift
line 409 at r4 (raw file):
extension RelayTests { /// Connect to a relay in the default country and city, get name and IP address of the relay the app successfully connects to. Assumes user is logged on and at tunnel control page. private func getDefaultRelayInfo() -> RelayInfo {
nit
Why was this moved to an extension ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @rablador)
ios/MullvadVPNUITests/Pages/TunnelControlPage.swift
line 213 at r4 (raw file):
func getCurrentRelayName() -> String { let server = app.staticTexts[.connectionPanelServer]
❌ Reference to member 'connectionPanelServer' cannot be resolved without a contextual type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @buggmagnet)
ios/MullvadVPNUITests/RelayTests.swift
line 409 at r4 (raw file):
Previously, buggmagnet wrote…
nit
Why was this moved to an extension ?
Keeps tests "clean", with helper functions and such outside.
ios/MullvadVPNUITests/Pages/TunnelControlPage.swift
line 213 at r4 (raw file):
Previously, buggmagnet wrote…
❌ Reference to member 'connectionPanelServer' cannot be resolved without a contextual type
Ack, I forgot to update it when chaniging name.
ios/MullvadVPN/View controllers/Tunnel/ConnectionView/ConnectionViewViewModel.swift
line 27 at r3 (raw file):
Previously, buggmagnet wrote…
nit
We had a team agreement in the past that we should refrain from doing these kind of things as it quickly make the codebase harder to read and to navigate in.I think it's okay for now, because there's only 1 layer of indirection, but I would like to encourage us to find a better way to do this next time.
We chould just move it to an update func if you like.
00fe32c
to
6d700b3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 8 of 8 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions
6d700b3
to
e004028
Compare
🚨 End to end tests failed. Please check the failed workflow run. |
With the changes to the connection view, UI automation needs to be adjusted to continue working.
Ensure that all tests use the new accessibility identifiers to exercise the connect view, by updating the accessibility tags.
When the changes here are applied, the new connection can be used in default builds, and remove any references to the old connect view.
This change is